Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Early support for @console_rule. #6088

Merged
merged 7 commits into from
Jul 17, 2018
Merged

Conversation

kwlzn
Copy link
Member

@kwlzn kwlzn commented Jul 10, 2018

Closes #6001

@kwlzn kwlzn force-pushed the kwlzn/moon_landing branch 2 times, most recently from be57d44 to f95c064 Compare July 10, 2018 17:06
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Kris, this looks great. My main surprise was the changes in GoalRunner... I figured that if the v2 pipeline was running, it would be long-completed by then.

@@ -153,73 +170,75 @@ def _setup_context(self):
context = Context(options=self._options,
run_tracker=self._run_tracker,
target_roots=target_root_instances,
v2_target_roots=target_roots,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that Context enters into the picture at all here is surprising to me. It seems like the context could/should just always be created after all v2 logic has already completed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

global_options = self._context.options.for_global_scope()
self._validate_workdir(global_options.pants_workdir)

# N.B. For daemon runs, console rules execute pre-fork.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that my thinking on this was that @console_rules always ran "first"... in the sense that they were sortof a different phase of the PantsRunner that didn't require the GoalRunner at all. Then the GoalRunner could (optionally) be created after they had run.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. refactored this a bit - let me know what you think now.

# Toggles v1/v2 `Task` vs `@rule` pipelines on/off.
register('--v1', advanced=True, type=bool, default=True,
help='Enables execution of v1 Tasks.')
register('--v2', advanced=True, type=bool, default=False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took a little while to grok the split here... I think it makes sense though. Would probably want to explain the heck out of it in the help.

@kwlzn kwlzn force-pushed the kwlzn/moon_landing branch 3 times, most recently from 9d34d32 to eb579d3 Compare July 14, 2018 09:30
@kwlzn kwlzn requested review from stuhood and jsirois July 15, 2018 08:00
Copy link
Contributor

@jsirois jsirois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 bug to fix, the other two comments are just ideas. I re-read the v2 Frontend HLD doc and these two ideas are decidedly off-plan, especially the Console product idea. So these are ignorable wrt landing on the moon, but otherwise perhaps useful longer term.

self._target_roots
)
except Exception:
logger.warn('Encountered unhandled exception {!r} during rule execution!')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing format call and arg.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

@staticmethod
def _make_goal_map_from_rules(rules):
goal_map = {}
goal_to_rule = [(rule.goal, rule) for rule in rules if getattr(rule, 'goal', None) is not None]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This getattr gets to the heart of things - it's not about @console_rule's (with the stress on console) - it's about user-facing goal names. One bit of what seems like conflation is @console_rule - which is just a rule made invokable by a user (from the console) - could be called @goal_rule or @exposed_rule or @cli_rule - and Console. You can imagine you'd wan't a CLI-invokable rule that has no (direct) Console interaction.

Copy link
Member

@stuhood stuhood Jul 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@foreground_rule is another name that was floated, since it's the "run in the foreground, exclusively" bit that makes it safe to give them console access.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, could be called any of those - but didn't feel strongly enough about it in any particular way beyond what we'd called them in the HLD.

I think from an end user approchability/grokability perspective, the @console_rule naming feels clearest to me to imply "a rule that runs with exclusive access to the terminal console". e.g. most new authors won't understand off the bat what an "@exposed_rule" is etc. ultimately, this is all a matter of conceptual overview and framing in the docs tho.

if either of you feel more strongly about the naming, happy to discuss on Slack. we can always change the naming later too, if deemed more appropriate to be referred to as e.g. @foreground_rule. but I like @console_rule, for the moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly about it.



def console_rule(goal_name, input_selectors):
output_type = GoalProduct.for_name(goal_name)
Copy link
Contributor

@jsirois jsirois Jul 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Console were a product type, inverting the moon-landing flow, alot of future goals for console handling might fall out a bit more naturally.

A sketch - here IO is installed as a singleton that is opaque with no methods to take advantage of in user code. In order for user rule code to create a Console (or InteractiveConsole (stdin)) or Logger, they must pass in an IO. Behind the scenes it would probably carry a Clock.

@rule(Console, [Select(IO), Select(BuildFileAddresses)], goal='list')
def fast_list(io, addresses):
  """A fast variant of `./pants list` with a reduced feature set."""
  console = Console(io)
  for address in addresses.dependencies:
    console.print_stdout(address.spec)
  return console

So now a @console_rule is just a @rule that produces a Console and has a user-facing CLI name (goal='list'). The console would have no handle to real io, it would just buffer data. The engine could then validate and aggregate and/or serialize CLI @rules and Console products, actually driving the Console with one real console.

If the native engine were modified slightly to handle nullary Get and rules returning a generator - ie: feed generator rules until StopIteration instead of assuming a single yielded value, this could be:

@rule(Stream(Console), [Select(BuildFileAddresses)], goal='list')
def fast_list(addresses):
  """A fast variant of `./pants list` with a reduced feature set."""
  console = yield Console(Get(IO))
  for address in addresses.dependencies:
    yield console.print_stdout(address.spec)

Where Console.print_stdout returned a Console.

Copy link
Member

@stuhood stuhood Jul 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I think that from my perspective, Select(IO) is approximately what Select(Console) does currently: ie, represents exclusive access to a console.

I'm not sure I see the advantage of the console being an output product when the other guarantees need to be met (exclusive, foreground access). Even in a world where we added back streaming of the console product with Stream(Console), the API would seem to be unnecessarily restrictive given that we know it "owns" the console while running.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, during design discussions I had initially proposed the idea of a Console as buffered virtual console in a similar vein to what you're suggesting (except without the Console/IO split - just wholesale copies) - but we'd decided against that in favor of more direct, exclusive access to accommodate a wider set of rule types to start.

I think for now, I agree with Stu that these are approximately equivalent - but I definitely see this iteratively evolving way beyond this current ~throwaway impl.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome... much cleaner than before.

Thanks Kris!

@kwlzn kwlzn merged commit 5f36f15 into pantsbuild:master Jul 17, 2018
stuhood pushed a commit that referenced this pull request Aug 7, 2018
### Problem

The ability to run a command continuously as files change has been a long-standing feature request, and something which is implemented in competing tools. It is very useful for low-latency compiler and test feedback.

### Solution

Builds atop @kwlzn 's work in #6088 and adds a `--loop` flag which re-runs all `@console_rule`s whenever input files have changed.

Adds a covering integration test (and refactors the pantsd test harness a bit to do so). 

### Result

```
./pants --enable-pantsd --v2 --no-v1 --loop list ${dir}::
```
... will continuously list the contents of a directory when files under the directory are invalidated.
CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018
### Problem

The ability to run a command continuously as files change has been a long-standing feature request, and something which is implemented in competing tools. It is very useful for low-latency compiler and test feedback.

### Solution

Builds atop @kwlzn 's work in pantsbuild#6088 and adds a `--loop` flag which re-runs all `@console_rule`s whenever input files have changed.

Adds a covering integration test (and refactors the pantsd test harness a bit to do so). 

### Result

```
./pants --enable-pantsd --v2 --no-v1 --loop list ${dir}::
```
... will continuously list the contents of a directory when files under the directory are invalidated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants